Repository navigation
chore(platform)!: update rust-dashcore (incl secp256k1 0.33) - #5307
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Final review complete — no blockers (commit 9774c72) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Phase 1 + Phase 2
Verified the supplied findings against head ad2b8c2, including the exact pinned upstream SPV implementation. Two migration regressions remain: successful SPV startup discards the runtime's client, and legacy WASM-DPP inherits an undeclared WebCrypto runtime requirement; the overlapping historical-storage test suggestions are consolidated below. Verification was static only: the supplied CI snapshot shows Kotlin build/tests passing, principal Rust and JS suites skipped, and PR Hygiene pending.
🔴 2 blocking | 🟡 1 suggestion(s)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large cross-cutting diff directly adapts signature and key-handling implementations in packages/rs-dpp/src/identity/identity_public_key/key_type.rs and packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs, alongside historical masternode processing and serialization, rather than merely bumping dependencies. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `Cargo.toml`:
- [BLOCKING] Cargo.toml:70: Adapt SpvRuntime to the newly non-blocking SPV run method
The pinned revision changes DashSpvClient::run() from waiting for shutdown to spawning an internal sync task and returning after startup succeeds. packages/rs-platform-wallet/src/spv/runtime.rs:374–381 still treats that return as shutdown: it removes self.client and clears the peer tracker. Successful startup therefore makes broadcasts fail with "client not started", progress queries return None, and quorum lookups lose access to the running client. SpvRuntime::stop() subsequently finds no client and joins only the completed startup wrapper, so it reports success without stopping the upstream task. That task retains a client clone, storage and event handlers, allowing sync and callbacks to continue after the runtime reports shutdown. Retain the client after successful startup and adapt shutdown tracking to the new upstream task ownership, including the bounded-stop behavior. Add regression coverage for successful startup followed by client queries and explicit stop.
In `packages/rs-dpp/Cargo.toml`:
- [BLOCKING] packages/rs-dpp/Cargo.toml:101-103: Propagate the new Node runtime requirement to legacy WASM-DPP
The new entropy requirement also reaches @dashevo/wasm-dpp through its DPP dependency. Its ECDSA signByPrivateKey path calls the pinned Core signer, whose secp256k1 0.33 implementation rerandomizes the signing context using rand 0.9 and getrandom 0.3. That backend calls globalThis.crypto.getRandomValues without getrandom 0.2's CommonJS crypto fallback, so ordinary Node 18 scripts without a WebCrypto shim now fail when signing. packages/wasm-dpp/package.json declares no Node minimum, its production loader in lib/index.ts installs no shim, and the legacy dash SDK still documents support for any Node version. The test bootstrap explicitly installs crypto.webcrypto, masking this production failure. Apply the declared Node >=20 migration to the legacy package and update its consumers' advertised requirement, or initialize WebCrypto in the production Node loader to preserve previous support. Cover the production loader without relying on the test bootstrap's shim.
In `packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs`:
- [SUGGESTION] packages/rs-drive-abci/src/execution/platform_events/core_based_updates/update_masternode_list/update_state_masternode_list/v0/mod.rs:546-550: Pin pre-migration bytes in the historical storage compatibility test
Both expected_bytes here and the actual bytes at lines 570–574 are generated with the migrated MasternodeV0 codec and current dependencies. This verifies that extended RPC records project to the same representation as legacy-shaped inputs, but it does not establish compatibility with previously persisted bytes: a shared encoding change would affect both sides and still pass. Preserving the shipped persisted layout is an explicit goal of this migration. Add a fixed fixture produced at base revision 723b09e63610057377ef4efc808476fae43e88ef, deserialize it through deserialize_masternode_entry, and reproduce it byte-for-byte through serialize_masternode_entry for a frozen historical protocol and PlatformVersion::latest(). Those helpers also exercise the versioned Masternode enum used by persistence, which this direct MasternodeV0 encoding test bypasses. No actual persisted-encoding incompatibility was established by this review.
lklimek
changed the title
lklimek
changed the title
lklimek
changed the title
lklimek
changed the title
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 2 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
the
waiting-bots
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At d9149f1, two integration blockers remain: the dependency update changes SPV lifecycle ownership and introduces a WebCrypto requirement without the corresponding runtime adaptations. The historical-storage suggestion is outdated because its targeted projection test was removed and the retained persistence code matches the updated base. Validation was static only; the supplied CI snapshot still had Rust workspace, JS, Docker, and Swift schema checks queued, with dedicated wallet tests skipped.
🔴 2 blocking
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🔴 Blocking: Adapt SpvRuntime to the newly non-blocking SPV run method
packages/rs-platform-wallet/src/spv/runtime.rs:379-381
At the pinned 314f1060 revision, DashSpvClient::run_locked starts an internally owned task, stores its handle and cancellation token in sync_loop, and returns Ok after startup. This unconditional cleanup therefore removes a successfully running client and clears its peer tracker. Broadcasts and quorum queries subsequently reject the missing client, progress queries return None, and stop() can report success after joining only the completed startup wrapper while upstream synchronization and event monitors remain active.
Shutdown also needs an ownership adaptation, not just client retention. The pinned upstream stop() takes SyncLoop out of its shared Option before awaiting the task, coordinator, network, and storage teardown. The direct timeout around c.stop() at runtime.rs:417 drops that future on expiry, abandoning the remaining teardown; retrying cannot recover it through the runtime's startup-wrapper handle. Retain the client after successful startup and own teardown in a tracked task that survives timeout and caller cancellation, with retries joining the same work and restart blocked until completion. The separately described #5311 fix is absent from this exact head and must land before or together with the migration.
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large cross-cutting migration directly changes cryptographic key generation in packages/rs-dpp/src/identity/identity_public_key/key_type.rs and BLS key validation, private-key serialization, and canonical node-ID derivation in packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs, beyond merely bumping dependencies. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:379-381: Adapt SpvRuntime to the newly non-blocking SPV run method
At the pinned 314f1060 revision, DashSpvClient::run_locked starts an internally owned task, stores its handle and cancellation token in sync_loop, and returns Ok after startup. This unconditional cleanup therefore removes a successfully running client and clears its peer tracker. Broadcasts and quorum queries subsequently reject the missing client, progress queries return None, and stop() can report success after joining only the completed startup wrapper while upstream synchronization and event monitors remain active.
Shutdown also needs an ownership adaptation, not just client retention. The pinned upstream stop() takes SyncLoop out of its shared Option before awaiting the task, coordinator, network, and storage teardown. The direct timeout around c.stop() at runtime.rs:417 drops that future on expiry, abandoning the remaining teardown; retrying cannot recover it through the runtime's startup-wrapper handle. Retain the client after successful startup and own teardown in a tracked task that survives timeout and caller cancellation, with retries joining the same work and restart blocked until completion. The separately described #5311 fix is absent from this exact head and must land before or together with the migration.
In `packages/rs-dpp/Cargo.toml`:
- [BLOCKING] packages/rs-dpp/Cargo.toml:102-103: Propagate the new Node runtime requirement to legacy WASM-DPP
(existing thread: https://github.com/dashpay/platform/pull/5307#discussion_r4195848422)
Legacy WASM-DPP's ECDSA signByPrivateKey path reaches DPP's signer::sign and the pinned Core recoverable signer. DPP enables secp256k1's rand feature, and secp256k1 0.33.1 rerandomizes its signing context through rand::random(). The getrandom 0.3 wasm_js backend enabled here calls globalThis.crypto.getRandomValues without the previous CommonJS crypto fallback. Node 18 scripts without global WebCrypto therefore fail during signing.
This head still declares no Node minimum in packages/wasm-dpp/package.json, installs no WebCrypto shim in its production loader, and advertises unrestricted Node support for the legacy SDK. Its test bootstrap explicitly supplies crypto.webcrypto, masking that production condition. The reported Node 22 smoke test validates the proposed supported environment but does not update the compatibility contract shipped here. Land #5310's package requirements, consumer documentation, and runtime alignment before or together with this migration, or preserve older environments through a production-loader shim. Keep production-loader coverage independent of the test bootstrap.
|
Both @thepastaclaw blockers addressed in separate PRs #5310 and #5311 |
|
📖 Book Preview built successfully. Download the preview from the workflow artifacts. Updated at 2026-10-14T00:43:51.754Z |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 3 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
At c0b4b97, two blockers are confirmed: the SPV lifecycle wrapper still treats successful startup as termination, and the root-only BLS patch does not protect external Cargo/WASM consumers. The Node runtime finding is fixed, and the historical-storage test finding no longer applies because the criticized test was removed. Verification was static; the supplied CI snapshot has Rust workspace and several JS/E2E checks pending, with the dedicated Rust wallet job skipped.
🔴 2 blocking
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The cross-cutting migration directly changes cryptographic and key-handling code, including RNG seeding in rs-dpp/src/identity/identity_public_key/key_type.rs and BLS key validation and node-ID construction in rs-platform-wallet/src/wallet/provider_key_at_index.rs, rather than merely bumping dependencies. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `Cargo.toml`:
- [BLOCKING] Cargo.toml:78-80: Make the WASM BLS compatibility fix reach external Cargo consumers
Cargo ignores a dependency workspace's [patch] table and does not inherit its lockfile. Consequently, an external workspace consuming dash-sdk or drive-proof-verifier from this Git revision does not receive this compatibility fix. Both reach DPP's bls-signatures feature, which enables blsful → blstrs_plus and dashcore/bls → dashcore-crypto → dash-pkc. blstrs_plus requires blst exactly 0.3.12, and blst's links constraint prevents selecting a second version for dash-pkc. In registry blst 0.3.12, aggregate_verify and fast_aggregate_verify are gated behind std, which its build script excludes on wasm32-unknown-unknown; the pinned dash-pkc calls both unconditionally in pkgs/pkc/src/bls/scheme_ietf.rs:157 and :176. Thus the workspace patch allows in-tree WASM builds while external consumers still encounter missing-method compilation errors. Make aggregate verification compatible in the owning dash-pkc implementation, or encode a compatible dependency in the actual dependency chain, then update the pins while preserving DPP's historical behavior. Moving the patch into another dependency's workspace would not fix external consumption.
- [BLOCKING] Cargo.toml:70: Adapt SpvRuntime to the newly non-blocking SPV run method
(existing thread: https://github.com/dashpay/platform/pull/5307#discussion_r4195848409)
At the pinned 314f1060 revision, DashSpvClient::run_locked() spawns the continuous sync task, stores its SyncLoop internally, and returns Ok after startup. SpvRuntime::run() still unconditionally removes self.client and clears the peer tracker at runtime.rs:379–381 when that call returns. Successful startup therefore disables broadcasts, progress queries, and quorum lookups; stop() then finds no client and reports success after joining only the completed startup wrapper, while upstream synchronization and callbacks can remain active. Shutdown also needs cancellation-safe ownership: upstream stop() takes the internal SyncLoop before awaiting its task, coordinator, network, and storage cleanup, but the current runtime drops that future on timeout. Retaining the client alone would leave teardown completion untracked after timeout or caller cancellation. Incorporate #5311's lifecycle adaptation with this pin, preserving a tracked teardown task and restart barrier until cleanup completes, and cover post-start queries, explicit shutdown, and timeout/cancellation retries.
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 2 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
Verified the findings against head 938b60f and the exact pinned upstream sources; two blockers remain after consolidating the duplicate SPV reports. The Node runtime requirement is satisfied, and the historical projection-test suggestion no longer applies. This was static verification only: the supplied CI snapshot shows successful JS/Docker builds and Kotlin checks, with Rust workspace tests running and JS/runtime suites queued.
🔴 2 blocking
2 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This cross-cutting migration directly changes cryptographic key generation and handling, including RNG seeding in packages/rs-dpp/src/identity/identity_public_key/key_type.rs and provider key derivation in packages/rs-platform-wallet/src/wallet/provider_key_at_index.rs, rather than merely updating dependency pins. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `Cargo.toml`:
- [BLOCKING] Cargo.toml:78-80: Make the WASM BLS compatibility fix reach external Cargo consumers
(existing thread: https://github.com/dashpay/platform/pull/5307#discussion_r4198966217)
This patch makes repository builds work, but Cargo does not inherit a dependency workspace's root [patch.crates-io] table. An external wasm32-unknown-unknown consumer enabling DPP's bls-signatures feature brings in blsful → blstrs_plus, whose manifest requires exactly blst =0.3.12. The migrated dashcore BLS feature also enables dash-pkc at e6402ced, whose scheme_ietf.rs calls fast_aggregate_verify and aggregate_verify unconditionally. Registry blst 0.3.12 gates both methods behind std, and its build script deliberately does not enable that configuration for this target. Consequently, the external dependency graph lacks the methods required by the migrated implementation, even though the workspace's patched builds pass.
The reported registry-versus-patched reproduction demonstrates why the workaround is necessary, not that it reaches downstream Cargo roots. Make compatibility effective through the normal dependency chain, or explicitly document the required consumer-root patch and its exact revision in the supported integration instructions. Validate that configuration from a separate Cargo root. Existing prebuilt npm artifacts are not affected by this dependency-resolution issue.
- [BLOCKING] Cargo.toml:70: Adapt SpvRuntime to the newly non-blocking SPV run method
(existing thread: https://github.com/dashpay/platform/pull/5307#discussion_r4195848409)
At this pin, DashSpvClient::run_locked spawns the synchronization task, stores its handle internally, and returns Ok(()) after startup. SpvRuntime::run still treats that return as termination: runtime.rs:379–381 removes self.client and clears the peer tracker. Successful startup therefore removes access to broadcasts, progress and quorum queries while upstream synchronization continues. A subsequent stop finds no client and joins only the completed startup wrapper; PlatformWalletManager::shutdown then records WorkerStatus::Ok without stopping the upstream task. That task retains the client and event handlers, so synchronization and callbacks can remain active after reported shutdown.
Client retention alone is insufficient: runtime.rs:417 directly times out c.stop(), while the pinned upstream stop takes its internal SyncLoop before awaiting teardown. Cancelling that future can discard shutdown ownership, and Platform's outer startup handle cannot establish that the internal task and remaining cleanup completed. Incorporate the #5311 lifecycle adaptation with this migration: retain the client after successful startup, preserve a tracked teardown task across timeout and caller cancellation, let retries join the same work, and prevent restart until teardown completes. The separately reported #5311 tests do not validate this head, whose runtime has no corresponding lifecycle change.
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 2 threads unresolved — resolve them. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The inspected cryptographic adaptations preserve signing preimages and canonical byte handling, and the incorporated Node requirements and SPV teardown changes resolve the earlier lifecycle blockers. Two blocking defects remain: external Cargo WASM consumers do not inherit the BLS compatibility patch, and clearing a constructed-but-not-running SPV client can leave its persistence worker writing to the cleared directory. This was a static review at the exact head; the supplied CI snapshot shows Kotlin checks passing, with Rust workspace validation, JS builds, Swift schema validation, and Docker builds still pending.
🔴 2 blocking | 🟡 2 suggestion(s)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large, cross-cutting diff directly adapts cryptographic and key-handling implementations in packages/rs-dpp/src/identity/identity_public_key/key_type.rs, packages/rs-platform-encryption/src/ecdh.rs, and wallet signing and key-derivation code, beyond merely bumping dependencies. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:620-623: Stop the constructor's persistence worker before clearing storage
A supported start(config) followed by clear_storage() without spawning the run loop leaves the original storage worker alive. At the pinned rust-dashcore revision 314f1060, DiskStorageManager::new() immediately starts a five-second persistence worker, but DashSpvClient::stop() stops storage only when sync_loop contains a running loop. This closure consequently returns without stopping that worker, and stop_locked drops the client before clear_storage_with reopens the same directory. DiskStorageManager has no Drop cleanup for its worker handle; the detached task retains the old storage Arcs while the directory lock is released. Its persistence loop continues creating storage subdirectories and can write still-dirty header segments into the cleared or restarted store. The previous live-client clear path cleared through the existing storage manager, which explicitly cancelled its worker. Stop the client's storage explicitly within the owned cleanup before dropping it, including cleanup after failed startup, and add a constructor-only clear regression that verifies the old worker cannot write after clearing.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:299-304: Separate retained client ownership from background running state
Retaining the client is necessary for safe teardown, but its presence no longer proves that synchronization is running. The pinned upstream run() returns after startup; a later sync-loop failure invokes on_error and schedules stop_failed(), without returning another error to this wrapper. finish_startup therefore leaves self.client populated after the upstream client has stopped. SpvRuntime::is_started() tests only that Option, and platform_wallet_manager_spv_is_running exposes the result as whether SPV is currently running. PlatformEventManager forwards the error without updating lifecycle state, so that query can remain true indefinitely after background synchronization terminates. Keep resource ownership separate from running status, using the upstream is_running() query or an explicit lifecycle signal, and cover successful startup followed by a background failure.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:999-1000: Add a regression that runs the real SPV startup wrapper
This is useful unit coverage for finish_startup, but it passes Ok(()) directly and installs an unrelated empty Tokio task. It never executes SpvRuntime::run() or creates the upstream SyncLoop, so it cannot detect a regression in the production startup-task wiring, and its stop assertion does not exercise running-client teardown. Add a separate regression that calls spawn_run_loop(), waits for the actual startup task to finish, then checks retained query access and explicit stop. The existing offline configuration restricts connections to an empty configured peer list; the pinned network manager supports that configuration without DNS discovery or peer connections, so this can exercise real startup without network-dependent testing.
In `Cargo.toml`:
- [BLOCKING] Cargo.toml:78-80: Make the WASM BLS compatibility fix reach external Cargo consumers
(existing thread: https://github.com/dashpay/platform/pull/5307#discussion_r4198966217)
Cargo applies this patch only when Platform is the consuming workspace root; an independent application depending on DPP or dash-sdk does not inherit it or Platform's lockfile. The ordinary dependency graph still combines blsful → blstrs_plus 0.8.18, which requires exactly registry blst 0.3.12, with dashcore → dashcore-crypto → dash-pkc. At the pinned base-sdk revision e6402ced, dash-pkc's scheme_ietf.rs unconditionally calls fast_aggregate_verify and aggregate_verify. Registry blst 0.3.12 gates both methods behind std, and its build script excludes that configuration on wasm32-unknown-unknown. External WASM consumers enabling BLS therefore encounter missing-method compilation errors, even though repository builds use the compatible patch. The reported patched reproducer establishes the implementation fix, but not its delivery to downstream consumers. Make the repair available through ordinary transitive dependency declarations, or explicitly document and support the required consumer-root override, and validate a standalone WASM consumer outside this workspace. This affects Rust-to-WASM consumers, not users of the already-built npm artifacts.
|
Waiting for bot review — coderabbitai not yet · thepastaclaw requested changes — dismiss the review or push a fix, 1 thread unresolved — resolve it. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The current head incorporates the Node-runtime, retained-client shutdown, and external-consumer BLS fixes, but one blocking SPV storage-worker defect and two lifecycle/testing suggestions remain. The storage-clear path can reopen the directory while its original persistence worker is still alive. Validation was static only; the supplied CI snapshot shows successful JS builds and Kotlin checks, with Rust workspace tests, Docker builds, and most JS checks pending and Rust wallet tests skipped.
🔴 1 blocking | 🟡 2 suggestion(s)
3 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large cross-cutting diff directly changes cryptographic key parsing, signature handling and canonical encodings in packages/rs-dpp/src/bls/bls_signatures.rs and packages/rs-dpp/src/bls/serde.rs, rather than merely updating dependency pins. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 15% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [BLOCKING] packages/rs-platform-wallet/src/spv/runtime.rs:620-623: Stop the constructor's persistence worker before clearing storage
(existing thread: https://github.com/dashpay/platform/pull/5307#discussion_r4205797515)
This cleanup stops only the upstream client before dropping it and reopening its storage directory. At the exact rust-dashcore pin, DiskStorageManager::new() immediately starts a five-second persistence task, but DashSpvClient::stop() reaches storage.stop() only when a SyncLoop exists. Therefore start(config) followed by clear_storage() without spawn_run_loop() releases the original directory lock without stopping its writer. The detached task retains the old storage components and path; persistence recreates directories and writes dirty segments, so clearing or restarting can race with stale writes. This PR replaces the base's live-client clear path, which cleared through the existing storage manager and cancelled that worker. Explicitly stop the original storage within owned teardown before releasing or reopening the directory, and ensure construction/startup failure cleanup cannot detach the same worker. Add a constructor-only clear regression that checks for writes after multiple persistence periods. The cleanup described in your #5322 reply is not present here: runtime.rs is unchanged since 7c73e983.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:186-188: Separate retained client ownership from background running state
(existing thread: https://github.com/dashpay/platform/pull/5307#discussion_r4205797527)
is_started() measures retained client ownership, but platform_wallet_manager_spv_is_running exposes it as synchronization liveness. The pinned upstream run() returns after startup; a later background failure cancels its running token, reports on_error, and schedules stop_failed() without returning through this runtime's completed startup wrapper. PlatformEventManager only forwards the error, so the retained client remains populated and the public running query can stay true after synchronization stops. Keep the client for teardown, but expose a separate running query backed by upstream is_running() and route the FFI export through it. Your real channel-failure regression tests the appropriate boundary, but neither that regression nor the separate query from #5322 appears in this head.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:999-1000: Add a regression that runs the real SPV startup wrapper
(existing thread: https://github.com/dashpay/platform/pull/5307#discussion_r4205797537)
This regression calls finish_startup(Ok(())) directly and installs an unrelated empty Tokio task. offline_runtime() constructs the client but never starts its upstream SyncLoop, so the assertions test constructor-level query access and helper behavior rather than production startup wiring or running-client teardown. The other spawn_run_loop() calls in this module exercise teardown refusal or an unstarted runtime, not successful startup. Add a separate offline regression that calls spawn_run_loop(), waits for its actual startup task, checks retained query access, explicitly stops the running client, and verifies restart. The existing empty restricted-peer configuration supports this without DNS discovery or peer connections. The production-wrapper regression linked in your #5322 reply has not been incorporated into the reviewed tree.
Move the eight rust-dashcore workspace pins from the head of rust-dashcore#1149 (now merged) to `dev` at d036d844, and the `dash-pkc` base-sdk pin to 33fcc59e so the lockfile keeps one base-sdk revision. Adapt to what changed upstream in between: - `signer::CompactSignature` is gone. Address-witness verification and the Core-signer paths use `sign_message::MessageSignature`. It accepts the same 65-byte signatures (header 27..=34, same recovery-id mapping) and `MessageSignature::new(sig, true).serialize()` yields the bytes `to_compact_signature(true)` did; only the parse error text differs. - `dashcore_rpc::Error::Secp256k1` is gone; drop its DAPI mapping. - ProRegTx / ProUpServTx carry `MasternodeNetInfo`. The wallet reads the primary routable address and keeps the `ip:port` rendering; version-3 platform ports and payout shares are not surfaced yet (TODO). - `WalletInfoInterface::truncate_above` is delegated to the Core wallet. - `WalletEvent::ChainTruncated` updates the displayed balance. It is not mirrored to the persister: the changeset cannot lower a height, sweep without a winner, or demote a confirmed record. Marked TODO(#5380). Document the removed BIP38 C exports in the changelog. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PSi8cX229kW2fmLxKW6ir4
dash-spv rewinds its storage and the wallets to a fork a peer announces before the competing branch is validated beyond its own header; only the best ChainLock bounds it. The fix belongs upstream and is tracked in dashpay/rust-dashcore#1103, so leave a TODO where the client is built. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PSi8cX229kW2fmLxKW6ir4
5d8438f to
1bc426d
Compare
|
I force-pushed this branch to reorganize its history: New layout (on
Why there's still one merge: #5359 upstreamed this branch's frozen vectors and part of #5320's BLS compatibility tests. With #5359 underneath, #5320's reviewed diff no longer applies unchanged, so v5.1-dev has to be merged after the sub-PRs. What's left to review beyond the sub-PRs: commit 1, the merge resolution, and the branch's own commits in the map below.
Open PRs based on this branch (#5150, #5323, #4844) need a rebase onto 🤖 Posted autonomously by Claude on behalf of pasta. |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
github-actions
Bot
added
waiting-bots
Moves the pin one commit forward on rust-dashcore `dev`, from d036d844 to its direct child 90359ed5, which adds rust-dashcore#1155: a special transaction payload is decoded only from transaction version 3, as Core does. Below version 3, non-zero type bytes stay raw and carry no payload. v5.1-dev already relies on that decoding through its own pin, so the branch needs it before the base can be merged without losing behaviour. Cargo.lock changes only the rust-dashcore source revision. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PSi8cX229kW2fmLxKW6ir4
`dpp::bls::PublicKey` parses the canonical identity encoding, as persisted state and shipped validation require, and only refuses it when a signature is verified. A caller that must not accept such a key at all, like a quorum key taken from a trusted source, had no way to ask. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PSi8cX229kW2fmLxKW6ir4
Brings in v5.1-dev at 1188304, which decodes Core v24 blocks and transactions (#5392) against rust-dashcore 6c5775cf. This branch stays on rust-dashcore `dev` at 90359ed5, which carries an equivalent of every commit 6c5775cf has over the common ancestor. Conflicts: - Cargo.toml, Cargo.lock: grovedb from v5.1-dev, rust-dashcore from this branch. The lock starts from v5.1-dev's and re-resolves only the rust-dashcore source; no other package takes a version that neither parent had. - CHANGELOG.md: both sides kept. The note that version-3 provider payloads are read through their pre-v3 fields is dropped, v5.1-dev reads them. - rs-drive-abci rpc/core.rs: `credit_pool_balance_from_coinbase` is the same on both sides; v5.1-dev's comment and tests are kept, they cover more. - rs-platform-wallet masternode record.rs, tracked.rs: v5.1-dev's version-3 handling (payout list, platform HTTP port from the address map) replaces this branch's pre-v3-only reading. - rs-platform-wallet masternode update_service.rs: this branch's `dpp::bls` imports. Ports the merge forces, because v5.1-dev added callers of the generic `dpp::bls_signatures` API this branch removed: - rs-sdk-trusted-context-provider: a quorum key from the trusted source is parsed with `dpp::bls::PublicKey`, which accepts the identity point, so it is refused through `is_identity()`; a test covers that key. - Tests in rs-drive-abci, rs-drive-proof-verifier and wasm-sdk sign with `dpp::bls::SecretKey`. Keys that committed vectors depend on are the scalars the previous backend derived from the same seeds, so the vectors are unchanged. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PSi8cX229kW2fmLxKW6ir4
…allet dash-spv tells the wallet the height of a chain lock it sees during the initial sync, before any chain lock is applied, so that spends observed at or below the synced height can be forgotten. `PlatformWalletInfo` forwarded `apply_chain_lock` but not `note_chain_lock_height`, whose trait default does nothing, so those spends stayed until the first chain lock was applied. <sub>🤖 Co-authored by [Claudius the Magnificent](https://github.com/lklimek/claudius) AI Agent</sub> Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PSi8cX229kW2fmLxKW6ir4
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The dependency, cryptography, persistence, and SPV lifecycle adaptations are present at the reviewed head, and no in-scope blocking defect remains. Four non-blocking wallet integration issues remain: initial-sync ChainLock notifications are not forwarded, native IPv6 endpoints are rendered ambiguously, v3 service updates can erase previously known endpoint fields, and quorum-type inputs are silently narrowed. Several broader persistence and upstream lifecycle limitations remain explicitly deferred and are recorded as follow-ups.
🟡 4 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Reject quorum types before narrowing them to u8
packages/rs-platform-wallet/src/spv/runtime.rs:371-375
The public SPV lookup accepts a u32 quorum type but casts it directly to u8. Inputs such as 256 and 257 therefore wrap to 0 and 1 and can query a different LLMQ type instead of returning an invalid-input error. This boundary is used by FFI and proof-verification integrations, so silently aliasing malformed input is unsafe and nondeterministic from the caller's perspective. Use a checked conversion before constructing LLMQType.
let quorum_type = u8::try_from(quorum_type).map_err(|_| {
PlatformWalletError::SpvError(format!("invalid quorum type: {quorum_type}"))
})?;
let llmq_type = LLMQType::from(quorum_type);
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 5: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 8: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This is a large cross-cutting dependency migration that directly changes cryptography and signature/key handling, consensus-facing serialization and peer-facing network data parsing across Rust, WASM, wallet, DPP, and FFI code, including files such as packages/rs-dpp/src/bls/, packages/rs-platform-wallet/src/masternode/, and packages/rs-platform-wallet/src/spv/runtime.rs. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/wallet/platform_wallet_traits.rs:164-166: Forward initial-sync ChainLock height notifications to the inner wallet
The pinned WalletInfoInterface adds note_chain_lock_height with a no-op default. dash-spv uses this callback for validated ChainLocks buffered during initial synchronization, but PlatformWalletInfo delegates truncate_above and the other ChainLock methods without forwarding it. The inner ManagedWalletInfo therefore does not receive the observed finality boundary until SyncComplete, so observed-spend entries are retained longer than necessary during a long initial sync. Delegate the callback and cover the pruning behavior through this wrapper.
In `packages/rs-platform-wallet/src/masternode/record.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/masternode/record.rs:28-36: Preserve bracketed rendering for native IPv6 provider addresses
provider_ip_port normalizes IPv4-mapped IPv6 addresses, but then formats the remaining address with format!("{}:{}"). A native IPv6 endpoint is consequently rendered as an ambiguous value such as 2001:db8::1:20001, unlike the ProRegTx path, which uses SocketAddr::to_string() and emits [2001:db8::1]:20001. Consumers parsing the documented endpoint string cannot reliably separate the host and port. Render the normalized address as a SocketAddr instead.
- [SUGGESTION] packages/rs-platform-wallet/src/masternode/record.rs:377-384: Do not erase previously exposed masternode fields on v3 service updates
When a v3 ProUpServTx is the latest service update, the current code unconditionally assigns service_address from primary_service_address() and platform_http_port from the legacy field. A v3 payload may have no legacy primary IPv4/IPv6 entry and may carry its platform port only in the extended address map, so both expressions can produce None. The update then clears endpoint data that was known from an earlier registration or service update, while advancing service_height. Preserve each existing field unless the update supplies a usable replacement, or extract the corresponding v3 extended-map values.
In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:371-375: Reject quorum types before narrowing them to u8
The public SPV lookup accepts a u32 quorum type but casts it directly to u8. Inputs such as 256 and 257 therefore wrap to 0 and 1 and can query a different LLMQ type instead of returning an invalid-input error. This boundary is used by FFI and proof-verification integrations, so silently aliasing malformed input is unsafe and nondeterministic from the caller's perspective. Use a checked conversion before constructing LLMQType.
Out-of-scope follow-up suggestions (4)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Persist the rollback represented by ChainTruncated — The ChainTruncated adapter still emits an empty CoreChangeSet, so durable transaction rows, restored outpoints, and sync heights can remain on the replaced branch after restart. This is a concrete durability gap, but the PR explicitly documents it and defers the ordered changeset and SQLite, SwiftData, and Room work to #5380/#5384.
- Follow-up: Land the shared truncation operation and truncate-to-persist-to-reload coverage before releasing a wallet build that relies on SPV fork recovery.
- Validate the competing branch before destructive SPV rewind — The pinned upstream dash-spv implementation can emit fork recovery before fully validating the competing branch, allowing a peer to trigger bounded wallet and header rewinds. The limitation is upstream, explicitly documented in this PR, and tracked as rust-dashcore#1103 rather than introduced by a Platform-side adaptation.
- Follow-up: Pin the upstream fork-recovery fix and add a regression proving invalid or insufficient-work competing headers cannot trigger a destructive rewind.
- Join upstream auxiliary SPV tasks before declaring teardown complete — dash-spv can leave detached stop_failed or fork-recovery helpers holding client and storage references after the tracked synchronization task and client stop complete. The resulting transient storage-lock window is acknowledged in the PR and tracked as #5381; the private helper tasks cannot be joined by this downstream adapter alone.
- Follow-up: Add an upstream completion barrier that joins all helper tasks before SPV stop reports success, with immediate clear and restart coverage.
- Stamp the v4 coinbase blob format in the SQLite schema — Version-4 coinbase records add a serialized field while this branch retains schema V018, so a build containing this migration but not stacked #5150 does not reject the new format through the schema gate. The PR explicitly identifies V019 from #5150 as the required follow-up and states that the two changes must land together.
- Follow-up: Land #5150's V019 migration and older-reader rejection before deploying this dependency update to persistent wallet databases.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The pinned dash-spv revision introduces a remotely triggerable fork-recovery path that can truncate wallet and chain state before the competing branch is validated; the local TODO documents the risk but does not mitigate it. The lifecycle adaptation also allows readiness to remain true after background synchronization stops because the retained client and cached peer snapshot are treated as sufficient. Other reviewed concerns are either fixed or explicitly deferred to tracked follow-up work.
🔴 1 blocking | 🟡 1 suggestion(s)
1 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Gate broadcast readiness on the retained client's running state
packages/rs-platform-wallet/src/spv/runtime.rs:271-272
The new lifecycle intentionally retains self.client after upstream synchronization fails, while is_running() becomes false. is_broadcast_ready() currently treats the retained client and the last cached PeerTracker snapshot as sufficient, and PeerTracker is only cleared during owned teardown. After a background failure, a stale nonempty peer snapshot can therefore make wait_until_ready() return true even though the upstream client has stopped; a subsequent broadcast is then rejected or cannot reach the network. Query the retained client's running state before reporting readiness, and apply the same guard to connected_peers().
async fn is_broadcast_ready(&self) -> bool {
let client_guard = self.client.read().await;
let Some(client) = client_guard.as_ref() else {
return false;
};
client.is_running() && !self.peer_tracker.snapshot().is_empty()
}
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality)
1 carried-forward finding(s) already raised on this PR; not re-posting as new inline comments.
Review provenance
Source: reviewer 1: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 4: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 7: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — This large cross-cutting diff changes cryptographic APIs, BLS and ECDSA signing/key handling, legacy public-key serialization, consensus-facing transaction payload decoding, proof verification, and wallet funds/key-management paths, including packages/rs-dpp/src/bls and packages/rs-platform-wallet/src/spv/runtime.rs. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:271-272: Gate broadcast readiness on the retained client's running state
The new lifecycle intentionally retains self.client after upstream synchronization fails, while is_running() becomes false. is_broadcast_ready() currently treats the retained client and the last cached PeerTracker snapshot as sufficient, and PeerTracker is only cleared during owned teardown. After a background failure, a stale nonempty peer snapshot can therefore make wait_until_ready() return true even though the upstream client has stopped; a subsequent broadcast is then rejected or cannot reach the network. Query the retained client's running state before reporting readiness, and apply the same guard to connected_peers().
In `Cargo.toml`:
- [BLOCKING] Cargo.toml:70: Validate the competing branch before allowing destructive SPV rewind
(existing thread: https://github.com/dashpay/platform/pull/5307#discussion_r4248258792)
The pinned dash-spv revision introduces a destructive fork-recovery path that can be triggered by an unmatched peer header whose parent is an eligible stored block below the current tip. In the pinned upstream code, fork detection occurs before the normal header-validation path, and the resulting lifecycle event truncates the wallet, block headers, filters, and blocks before the competing branch has been checked for proof of work, expected difficulty, or superior chain work. A malicious connected peer can therefore force repeated shallow rewinds and resynchronization without mining a valid replacement chain. The TODO at packages/rs-platform-wallet/src/spv/runtime.rs:165-170 records the limitation but does not prevent the rewind. Pin a repaired upstream revision or apply an equivalent patch that validates branch eligibility before emitting the destructive fork event, and retain a regression proving that fabricated unmatched headers leave wallet and storage state unchanged.
Out-of-scope follow-up suggestions (3)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Persist the rollback represented by ChainTruncated — WalletEvent::ChainTruncated still produces an empty CoreChangeSet, so persisted transaction rows, restored outpoints, confirmations, and synchronization heights can remain on the replaced branch after restart. This is a concrete wallet consistency gap introduced by the newly usable rewind path, but the PR explicitly excludes the cross-backend persistence migration and tracks it in #5380/#5384.
- Follow-up: Land the ordered truncation changeset and SQLite, SwiftData, and Room persistence work before releasing a wallet build that enables this rewind behavior.
- Join upstream auxiliary tasks before declaring SPV teardown complete — The pinned upstream client can leave detached failure or fork-recovery helpers holding client and storage references after stop reports completion, creating a transient directory-lock failure for immediate clear or restart. The helper ownership is private to dash-spv and the issue is tracked separately as #5381.
- Follow-up: Adopt the upstream lifecycle fix and add an immediate clear-and-restart regression covering failure and fork races.
- Stamp the new v4 coinbase blob format in the SQLite schema — Version-4 coinbase records add a serialized field while this branch retains SQLite schema V018, so an older binary can pass the schema gate and fail while decoding a newer transaction blob. The PR explicitly leaves the V019 migration to stacked #5150 and documents the interim compatibility window.
- Follow-up: Land #5150's V019 migration and older-reader rejection coverage with any release that writes version-4 coinbase records.
|
Waiting for bot review — coderabbitai not yet · thepastaclaw not yet. Wait for the missing reviews, or a writer can post |
The blocking finding is the upstream dash-spv fork-recovery behaviour tracked as dashpay/rust-dashcore#1103. It is not introduced by a Platform-side change, is documented in this PR (TODO in spv/runtime.rs and the known limitations), and the PR owner accepted the risk until the upstream fix can be pinned. The readiness suggestion is recorded under known limitations.
This reverts commit 835f6af. That commit worked around Test Suite failures after a quorum rotation: on a "Quorum not found in cache" proof error it waited 65 seconds and retried the proved read once. The failures were never caused by this branch. v5.1-dev failed the same way before #5404, and the workaround needed nothing from the rust-dashcore migration. The causes are now fixed at the source and reach this branch through the v5.1-dev merge (#5404): - #5313: the SDK, including wasm-sdk, fetches a missing quorum key from the trusted quorum service and verifies the response again, instead of banning every node. - #5354: dashmate reloads the local quorum list every second rather than every 60 seconds. - quorum-list-server v0.8.0 lists every quorum Platform may still sign with (dashpay/quorum-list-server#16). #5404's Test Suite passed without the retry. Keeping a fixed sleep keyed on error text would only hide a regression of those fixes. The standalone copy, #5420, was closed for the same reason. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I pushed Why: the retry worked around Test Suite failures this branch didn't cause.
#5404's Test Suite passed without the retry. A fixed 65 s sleep keyed on error text would only hide a regression of those fixes. The standalone copy, #5420, was closed for the same reason. The two test-suite files are back to their 🤖 Posted autonomously by Claude on behalf of pasta. |
|
Waiting for bot review — coderabbitai not yet · thepastaclaw ✓. Wait for the missing reviews, or a writer can post |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Re-review — Final validation — Phase 1 + Phase 2
The migration preserves the reviewed cryptographic, serialization, dependency, and storage compatibility behavior. Three actionable documentation and lifecycle issues remain: broadcast readiness can be reported after background SPV failure, the stop timeout does not cover lock acquisition or in-flight client operations, and Swift documentation describes teardown behavior that the native implementation no longer provides. Other reported concerns are explicitly deferred to stacked or upstream follow-ups.
🟡 3 suggestion(s)
2 finding(s) not shown inline (the lines are not part of this PR's diff)
🟡 Suggestion: Gate broadcast readiness on the retained client's running state
packages/rs-platform-wallet/src/spv/runtime.rs:271-272
The runtime intentionally retains the client after startup so it can be queried and stopped, but client ownership no longer proves that synchronization is running. After a background sync failure, is_running() becomes false while the client and the last peer snapshot can remain present; wait_until_ready() can therefore return true and the following broadcast is rejected by the stopped client. Include the existing running-state signal in this readiness predicate.
async fn is_broadcast_ready(&self) -> bool {
self.is_running() && !self.peer_tracker.snapshot().is_empty()
}
source: muse-spark-1.3-contributor (phase1-reviewer: general, rust-quality); gpt-6.1-sol (phase2-reviewer: general, architecture-layering)
🟡 Suggestion: Update Swift SPV stop documentation for non-aborting teardown
packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift:332-337
The current native implementation waits for the shutdown lock and in-flight client accesses before starting the bounded teardown wait. Once teardown starts, it waits up to 15 seconds for the owned task and returns shutdownIncomplete; it no longer has a separate 15-second client-stop budget or a two-second abort grace, and it never aborts upstream work. The Swift documentation therefore gives callers an incorrect approximately-32-second maximum and incorrectly promises abort escalation.
/// After any in-flight client access releases its read lock, native teardown waits
/// up to 15 s for the owned teardown task. It never aborts upstream work: if that
/// task is still running, this method throws `shutdownIncomplete` and keeps the
/// task tracked for a later stop. Waiting for the shutdown lock and in-flight
/// client operations is not separately bounded, and an in-flight broadcast may
/// add its acceptance timeout. Keep this call off the main actor.
source: gpt-6.1-sol (phase2-reviewer: general)
Review provenance
Source: reviewer 1: muse-spark-1.3-contributor (agent: phase1-reviewer, role: general); reviewer 2: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 3: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 4: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 5: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 6: muse-spark-1.3-contributor (agent: phase1-reviewer, role: rust-quality); reviewer 7: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 8: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); reviewer 9: gpt-6.1-sol (agent: phase2-reviewer, role: general); reviewer 10: gpt-6.1-sol (agent: phase2-reviewer, role: architecture-layering); reviewer 11: gpt-6.1-sol (agent: phase2-reviewer, role: ffi-engineer); reviewer 12: gpt-6.1-sol (agent: phase2-reviewer, role: platform-versioning); reviewer 13: gpt-6.1-sol (agent: phase2-reviewer, role: rust-quality); reviewer 14: gpt-6.1-sol (agent: phase2-reviewer, role: security-auditor); final verifier: gpt-6.1-sol (agent: sol-verifier, role: final-verifier)
- Triage:
criticalbygpt-6.1-sol(effort low) — The large diff changes critical cryptographic and peer-facing serialization surfaces, including BLS and secp256k1 APIs, identity and quorum key handling, and legacy PlatformNodeId and coinbase payload encoding in packages/rs-dpp and related wallet/consensus code. - Phase 1 reviewers:
muse-spark-1.3-contributor— general (completed, effort xhigh); agentphase1-reviewer,muse-spark-1.3-contributor— rust-quality (completed, effort xhigh); agentphase1-reviewer - Phase 1 model:
muse-spark-1.3-contributor— not quota-gated; passed overgemini-3.8-flash-high(antigravity below 15% reserve: weekly 14% left, 5h 100% left),glm-5.3-flash(not used above high effort; tier asks max) - Single stage: Phase 1 and Phase 2 reviewed this head side by side, with no blocker gate between them (triage tier)
- Fresh final gate: an independent Phase-2 review ran after iterative findings were reconciled
- Fresh verifier:
gpt-6.1-sol— final-verifier; agentsol-verifier - Phase 2 reviewers:
gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— general (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— architecture-layering (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— ffi-engineer (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— platform-versioning (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— rust-quality (completed, effort xhigh); agentphase2-reviewer,gpt-6.1-sol— security-auditor (completed, effort xhigh); agentphase2-reviewer
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/rs-platform-wallet/src/spv/runtime.rs`:
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:271-272: Gate broadcast readiness on the retained client's running state
The runtime intentionally retains the client after startup so it can be queried and stopped, but client ownership no longer proves that synchronization is running. After a background sync failure, `is_running()` becomes false while the client and the last peer snapshot can remain present; `wait_until_ready()` can therefore return true and the following broadcast is rejected by the stopped client. Include the existing running-state signal in this readiness predicate.
- [SUGGESTION] packages/rs-platform-wallet/src/spv/runtime.rs:418-424: Bound SPV stop while waiting for in-flight lifecycle operations
`SPV_STOP_TIMEOUT` only wraps the teardown task after `stop_locked` has acquired the shutdown mutex, taken the client write lock, and spawned the teardown task. A concurrent start can hold the shutdown mutex, while a client query or broadcast can hold the client read lock across an awaited operation; those waits occur before the timeout begins and can block `stop()` indefinitely. Since the FFI shutdown path waits for stop, a stalled query or lifecycle operation can prevent destruction from returning. Apply a cancellation-safe deadline to admission and lock acquisition, or otherwise allow shutdown to report a bounded failure without waiting indefinitely.
In `packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift`:
- [SUGGESTION] packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift:332-337: Update Swift SPV stop documentation for non-aborting teardown
The current native implementation waits for the shutdown lock and in-flight client accesses before starting the bounded teardown wait. Once teardown starts, it waits up to 15 seconds for the owned task and returns `shutdownIncomplete`; it no longer has a separate 15-second client-stop budget or a two-second abort grace, and it never aborts upstream work. The Swift documentation therefore gives callers an incorrect approximately-32-second maximum and incorrectly promises abort escalation.
Out-of-scope follow-up suggestions (5)
These are valid observations, but they are outside this PR's scope and should be handled in separate issues or author/maintainer-requested PRs rather than blocking this review.
- Persist the rollback represented by ChainTruncated —
WalletEvent::ChainTruncatedstill produces an emptyCoreChangeSet, so persisted transaction rows, restored outpoints, confirmation state, and sync heights can remain on the replaced branch after restart. This is a concrete wallet-integrity gap, but the PR explicitly excludes the persistence/accounting migration and documents the repair as #5380/#5384 stacked on this dependency update.- Follow-up: Land the ordered truncation changeset and atomic SQLite, SwiftData, and Room persistence changes before releasing a wallet that ships this SPV rollback path.
- Validate competing SPV branches before destructive rewind — The pinned upstream dash-spv revision can begin fork recovery from a peer-supplied header before validating the competing branch's proof of work or chain work. The behavior is upstream-owned, explicitly marked with a TODO, and tracked as rust-dashcore#1103 rather than being repairable in this wrapper migration.
- Follow-up: Pin a dash-spv revision containing the upstream fork-recovery validation fix and add an invalid-header regression.
- Join detached upstream SPV helper tasks before reopening storage — Upstream fork and failed-sync helper tasks can retain client and storage references after the locally awaited stop completes, allowing an immediate clear or restart to encounter a directory lock. Those helpers are private to dash-spv, and the limitation is already tracked separately as #5381.
- Follow-up: Add an upstream ownership barrier that joins all helper tasks before dash-spv reports stop completion.
- Reject quorum types before narrowing them to u8 —
get_quorum_public_key_with_statusstill casts the publicu32quorum type directly tou8, so values above 255 alias another LLMQ type. This predates the migration and is explicitly documented as a separate follow-up rather than being introduced by the dependency update.- Follow-up: Validate the conversion with
u8::try_fromat the wallet-runtime API boundary.
- Follow-up: Validate the conversion with
- Gate the new transaction-record blob format with a SQLite schema version — Version-4 coinbase records add a serialized field while this branch remains at schema V018. The PR explicitly defers the V019 schema gate to stacked #5150, which supplies the migration and older-reader rejection.
- Follow-up: Land #5150's V019 migration together with this pin before enabling the forward-format wallet release.
| /// Stop SPV sync, waiting up to 15 seconds for startup and teardown. | ||
| /// | ||
| /// **Every phase after taking the client is bounded** — `stop` runs on the | ||
| /// path [`PlatformWalletManager::shutdown`](crate::manager::PlatformWalletManager::shutdown) | ||
| /// and therefore the FFI's `destroy` must return through, so a wedged | ||
| /// host callback has to surface as an error rather than hang teardown: | ||
| /// the client stop is capped at [`SPV_CLIENT_STOP_BUDGET`], the run-loop | ||
| /// join at [`SPV_STOP_TIMEOUT`] with an [`SPV_ABORT_GRACE`] post-abort | ||
| /// confirmation. A run loop that outlives all of that is **re-parked**, | ||
| /// not detached, so a teardown retry re-joins it — and the error return | ||
| /// keeps it out of a clean shutdown verdict. | ||
| /// A timeout or cancelled caller leaves teardown tracked for the next stop. | ||
| /// Restart stays blocked until a stop confirms completion. Client queries | ||
| /// already in flight finish before teardown starts; this wait is unbounded. | ||
| /// | ||
| /// Taking the client is not bounded: it waits for the client's read | ||
| /// guards, and [`broadcast_transaction_and_wait`](Self::broadcast_transaction_and_wait) | ||
| /// holds one for its whole acceptance wait (the caller's timeout). That | ||
| /// keeps a broadcast on a live client and the data directory free once | ||
| /// `stop` returns, at the cost of a stop waiting for the broadcast. | ||
| /// # Errors |
There was a problem hiding this comment.
⏭️ Deferred (maintainer decision; not blocking) at
9774c72d; see the replies below.
🟡 Suggestion: Bound SPV stop while waiting for in-flight lifecycle operations
SPV_STOP_TIMEOUT only wraps the teardown task after stop_locked has acquired the shutdown mutex, taken the client write lock, and spawned the teardown task. A concurrent start can hold the shutdown mutex, while a client query or broadcast can hold the client read lock across an awaited operation; those waits occur before the timeout begins and can block stop() indefinitely. Since the FFI shutdown path waits for stop, a stalled query or lifecycle operation can prevent destruction from returning. Apply a cancellation-safe deadline to admission and lock acquisition, or otherwise allow shutdown to report a bounded failure without waiting indefinitely.
source: gpt-6.1-sol (phase2-reviewer: general, architecture-layering, ffi-engineer, platform-versioning, rust-quality, security-auditor)
There was a problem hiding this comment.
Not changed here. This is the documented contract of the stop path, not an oversight: SpvRuntime::stop says "Client queries already in flight finish before teardown starts; this wait is unbounded", and the 15 s budget is stated for startup and teardown only. That lifecycle came in with #5311 and #5322, which were reviewed on their own and merged into this branch; this PR is the dependency migration and does not revisit it.
Bounding admission and lock acquisition is a real follow-up, but it is a design change (what a timed-out stop leaves behind while a query still holds the client) rather than a fix to fold into the pin update. The remaining teardown window is tracked in #5381.
🤖 Co-authored by Claudius the Magnificent AI Agent
There was a problem hiding this comment.
I verified that stop explicitly documents the unbounded in-flight-query wait and that its locking/timeout sequence is inherited from #5311/#5322, so I accept deferring admission deadlines to a lifecycle follow-up rather than requiring that design change in this dependency migration.
Noted as intentionally deferred; I will not press it further here.
PastaPastaPasta
left a comment
PastaPastaPasta
left a comment
There was a problem hiding this comment.
I spent a good bit of time on this and reviewing and improving it; I think this is ready to merge.
|
@coderabbitai review No review for |
5 participants
Basic explanation
What this does: Pins rust-dashcore to
devat90359ed58a993bab0d4e267b4029bab132a56532, which includes rust-dashcore PR #1149, restoring legacy BLS public-key and canonical PlatformNodeId serialization and preserving the pre-v4 coinbase payload Serde layout. This is a port of #4932's migration tov5.1-dev.Value: Gives #5150 a separately reviewable dependency base, so its diff focuses on wallet history and persistence repairs. #4932 and its existing dependent PRs remain unchanged.
Risks: This changes public Rust cryptography APIs and introduces a WebCrypto runtime requirement for WASM; the Node.js >=22 package requirements are being split into separate PR #5310. The merged base supplies the versioned Core v24 behavior. The SPV lifecycle and Node runtime requirements are addressed in stacked #5311 and #5310, which must land with this migration.
Issue being fixed or feature implemented
Port of #4932 from
v5.0-devtov5.1-dev, retaining its commit ancestry. Dependency base for #5150; no SQLite accounting migration, wallet history replay, or SwiftData accounting repair is included here.What was done?
Port the secp256k1 0.33 API adaptations and WASM random-source configuration from chore(platform)!: bump rust-dashcore to 719de34b (secp256k1 0.33) #4932. Node.js package requirements and related README changes are excluded from this PR and will be raised directly to >=22 in a separate PR.
Pin all eight rust-dashcore workspace dependencies and all 13 upstream lockfile packages to
devat90359ed58a993bab0d4e267b4029bab132a56532. This revision also carries rust-dashcore feat: adapt for web extension #1155 (a special payload is decoded only from transaction version 3), whichv5.1-devrelies on since fix(platform)!: decode Core v24 blocks and transactions #5392. Cargo.lock uses the same revision for every upstream crate. Thedash-pkcbase-sdk pin moves to33fcc59e1868c803f9608e0e78ee8bddf3abcee7, the revision that rust-dashcoredevuses, so the lockfile keeps a single base-sdk revision.Adapt to the upstream changes that
devadds on top of feat: fetch data contract history endpoint #1149: compact ECDSA signatures go throughdashcore::sign_message::MessageSignature(theCompactSignaturetrait is removed); DAPI drops its mapping for the removeddashcore_rpc::Error::Secp256k1; the masternode record, tracking and service-update code is the onev5.1-devgained with fix(platform)!: decode Core v24 blocks and transactions #5392, built here againstdev's API;WalletInfoInterface::truncate_aboveandnote_chain_lock_heightare delegated to the Core wallet; the trusted context provider keeps rejecting an identity-point quorum key through the newdpp::bls::PublicKey::is_identity();WalletEvent::ChainTruncatedupdates the displayed balance.Adapt
wasm-dpp2private-key hex decoding to the removal ofdashcore_hashes::hex, using its existinghexdependency.Update the native wallet FFI node-ID helper to the explicit
EddsaPkHashconversion, preserving canonical SHA256-prefix bytes and the C ABI. A fixed-vector test and invalid-input/no-write checks cover the exported function.Adapt Core crypto, SPV and spent-height query APIs. Use the shared BLS backend through
dpp::bls, convert Ed25519 byte keys through the public operational-key API, preserve canonical node-ID bytes, and read owned masternode snapshots through the public SPV queries. The branch includes the SPV lifecycle fixes from fix(platform-wallet): retain SPV client and track shutdown through cancellation #5311 and fix(platform-wallet)!: tell a retryable SPV stop from one that needs a restart #5322.Remove DPP's
core_key_wallet_bip_38and SDK'score_key_wallet_bip38forwards because upstream removed BIP38 support. Adapt raw hash extraction and coinbase test fixtures to the newer upstream API.Incorporate
v5.1-devthrough054563c73e, including test(platform): freeze historical storage and signing vectors #5359's frozen vectors, common historical generation script and encryption test coverage in CI. Retain the migrated test APIs and additional BLS assertions. Keep the Core v24 identity, payout, port-resolution and persistence implementations from feat(platform)!: support Core v24 masternode identities at protocol version 14 #5227/feat(drive-abci)!: resolve Core v24 platform ports at protocol version 14 #5228 unchanged relative to the base. Remove this PR's superseded strict masternode conversions and legacy projection adapters.Use the shared
dash-pkcBLS backend with registryblst0.3.17;blsful,blstrs_plus, and the workspaceblstpatch are removed. DPP'sbls-signatures,core_verificationandcore_quorum_validationfeatures directly requireblst = "0.3.17"(>=0.3.17,<0.4), so external consumer lockfiles cannot retain incompatible 0.3.12. A TODO removes this temporary constraint once the pinned rust-dashcore graph enforces a compatible minimum.Isolate quorum-key persistence from crypto Serde with an explicit fixed-48-byte storage adapter. Preserve V1 current keys and V2 current/previous keys, with no disk-format change or migration. Frozen fixtures generated by pre-migration commit
7c73e983b4cover quorum records, full saved-state records and checkpoint state.In-place changes to shipped generations
The existing generation methods receive mechanical secp256k1 API adaptations with unchanged digest construction, signature rules and canonical byte representations. Masternode processing, persistence and protocol-version tables now match the base branch; the historical/PV14 boundary comes from #5227/#5228. No additional consensus behavior change is intended by the local API adaptations.
choose_quorum/v0(both selection helpers) andsignature_verification_quorum_set/v0/quorums.rskeep their score bytes unchanged for all selecting protocol versions (1–14): the oldFrom<Hash> for [u8; 32]andHash::to_byte_array()both return the same inner array without reversal. Existing quorum-selection tests cover the adapted code.QuorumForSavingV1retains its exact 48 compressed key bytes without a length prefix for every protocol version that reads or writes it. The adapter uses the same validated DPP key parser and leaves the hash, index, quorum ordering and metadata unchanged. V1's C++-encoded previous quorums remain unchanged; V2 applies the adapter to both lists. Historical byte-for-byte fixture tests verify this storage-only refactor; it changes neither consensus outputs nor the disk format.Address-witness verification (
PlatformAddress::verify_bytes_against_witness) and the Core-signer path (sign_with_core_signer) switch from the removedCompactSignaturetrait toMessageSignature. The accept/reject set is unchanged: both parsers require 65 bytes with a header in 27..=34 and map the header to the same recovery id and compression flag, andMessageSignature::new(sig, true).serialize()produces the bytesto_compact_signature(true)did. Only the text of the parse error differs; it reachesInvalidStateTransitionSignatureError.message, not the error code. The frozen signing vectors from #5359 pass unchanged.Merge order
This branch pins rust-dashcore
dev; rust-dashcore PR #1149 is merged there.Land this migration together with the Node runtime requirements in #5310 and SPV lifecycle adaptation in #5311. #5150 remains stacked above this PR, but must retain or restore dashpay/rust-dashcore#1112 separately: that PR is based on the
devrevision pinned here, and #5150 uses its additional API. #5307 itself does not require #1112.How Has This Been Tested?
Validation of the #5359 integration and Core-only BLS constraint (
674465874a), retaining #5366 atf29c092fcf:blst 0.3.12lockfiles. After the fix,cargo update -p dppselects 0.3.17, forcing 0.3.12 is rejected, and each Core/BLS dependency graph passes WASM Clippy.state_transitionimports whenstate-transitionsis disabled; those builds are not claimed green.Validation after merging
v5.1-dev(118830417e), on rust-dashcoredev(90359ed5), Platform commit912e06b19b:--workspace --all-targets --all-features --locked -- --no-deps -D warnings).platform-walletandplatform-wallet-storage: 2,235 tests passed, including the version-3 masternode tests and the stored-transaction-row fixture, which is unchanged.drive-abcirpc::core(5),voting_default_limitsandnonunique_merged_proofagainst their committed corpus and vectors (12), the trusted context provider and proof verifier suites, and thewasm-sdkcontext-provider and broadcast tests (11).wasm-sdktests also type-check forwasm32-unknown-unknown.drive-abci,rs-dapi,rs-sdkand strategy test suites; CI covers them.Earlier validation on rust-dashcore
dev(d036d844), Platform commit9183842643:--workspace --all-targets --all-features --locked -- --no-deps -D warnings), before three test-only additions;platform-walletwas linted again after them.dpp: 5,524 tests passed, including the frozen BLS, historical hash/signature and DashPay ciphertext vectors and the address-witness tests.platform-wallet,platform-wallet-ffi,platform-wallet-storage,rs-unified-sdk-jni: 2,947 tests passed.platform-encryption,rs-dapi,simple-signer: 353 passed.drive-abciplatform_types(quorum and saved-state storage): 126 passed.WalletEvent::ChainTruncated: no spend is reported, no payment hook is driven, and nothing is persisted yet.Validation on rust-dashcore PR #1149 (
8fe0a381), Platform commitf475f72a11:62 targeted tests passed: 8 frozen BLS tests, 4 historical transaction/hash/signature tests, 4 DashPay encryption tests, and 46 quorum/storage tests including historical saved states and checkpoints.
Native all-target/all-feature Clippy passed for
dpp,platform-encryption,drive-abci,platform-wallet,platform-wallet-storage,dash-sdk, andwasm-dpp2with--locked -- --no-deps -D warnings.wasm-dpp2library Clippy passed onwasm32-unknown-unknownwith all features. The additional wasm32 test-harness check fails onuse super::TS_TYPESinsrc/group/token_event.rs:149; that file is unchanged from the base. Native test-target linting passes. JavaScript/browser runtime tests were not rerun locally.Formatting, whitespace and pin-consistency checks passed; unrelated lockfile packages are unchanged.
External DPP consumer regression on
069a5cab6a: an existing lockfile selectedblst0.3.12 before the constraint;cargo update -p dppnow selects 0.3.17. Fresh resolution also selects 0.3.17, and explicitly selecting 0.3.12 is rejected. WASM all-target Clippy passes withbls-signaturesandstate-transitions; DPP formatting passes.cargo-macheteis unavailable locally; CI performs that check.Proof-verifier regression: all nine unit tests and the full package lint pass in CI. Four new cases cover delayed quorum publication/address availability, persistent failure, affected-state proofs, and invalid signatures. The main Test Suite passes with 95 tests; both browser suites and package functional tests pass on
c1c7b095c2.Validation on
e6873e95:platform-wallet-ffiregression tests and all-target/all-feature Clippy forplatform-wallet-ffi,rs-unified-sdk-jniandrs-unified-sdk-ffi.drive-abciall-target/all-feature Clippy, formatting and whitespace checks passed after the quorum adapter change.platform-wallet,platform-wallet-storageanddrive-abci: provider-key vectors, masternode tracking and service updates, SPV lifecycle and peer classification, quorum selection, coinbase credit-pool decoding, SQLite persistence and buffer semantics.platform-wallet,platform-wallet-storage,dash-sdkanddrive-abciwith--all-targets --all-features --locked --offline -- --no-deps -D warnings.CI on
c1c7b095c2passes Rust workspace tests, Kotlin, JS/WASM builds and tests, Docker builds, browser suites, Dashmate E2E and package functional tests (45 successful checks). Swift compiled the Rust library but failed when copying it:No space left on deviceon self-hostedmac-runner-pasta. Swift remains unverified on this head until runner disk space is restored and its job is rerun.Outstanding review blockers
WASM Node runtime requirement: Node version declarations have been removed from this PR at the author's request. PR #5310 supplies the Node >=22 package requirements, documentation and matching Docker runtime; this dependency must be landed together with the migration.
Quorum storage is covered by frozen pre-migration records. These fixtures do not establish compatibility for every historical wallet transaction or masternode record layout.
Breaking Changes
Public Core/secp256k1 Rust types and methods change; Platform consumers are adapted here. The migrated WASM entropy backend requires global WebCrypto; the Node.js >=22 minimum and related package/documentation changes are handled in a separate PR. The unified C API loses
bip38_encrypt_private_keyandbip38_decrypt_private_key(unsupported-operation stubs that upstream removed); direct C and Swift callers must drop their calls. The rest of the FFI ABI is unchanged. Upstream restores legacy BLS public-key hex serialization and canonical PlatformNodeId bytes, and preserves the pre-v4 coinbase Serde shape. No database migration is included; compatibility with every historical record layout, or with records written by the intermediate raw-BLS/reversed-node-ID implementation, is not established by this pin. DPP'score_key_wallet_bip_38and SDK'score_key_wallet_bip38features are removed because upstream removed BIP38.Known limitations
WalletEvent::ChainTruncated) corrects the in-memory wallet but is not mirrored to persisted wallet state. After a restart the stored wallet stays on the replaced branch: transactions of dropped blocks load as confirmed, the coins the truncation restored load as spent, and scanning resumes above the stored sync height, so the replacing blocks between the fork and the old tip are not scanned again until a rescan. While the wallet runs, a coin a truncation hands back is not fenced against a second spend. ChainLocks keep the rewind shallow, and the stored state is no worse than before this change, which never rewound. Tracked in platform-wallet: persist chain truncation (reorg) reported by key-wallet #5380; fix(platform-wallet)!: persist chain truncations so a stored wallet follows a reorg #5384 stores the truncation and re-arms the fence, and should merge before a wallet release that ships this pin.SpvRuntime::get_quorum_public_keynarrows itsu32quorum type withas u8, so an out-of-range value selects another LLMQ type instead of failing. The cast predates this PR onv5.1-dev.wait_until_readystill reports readiness from the retained client and the last peer snapshot. A broadcast issued then is rejected with an error instead of waiting for a restart.spv/runtime.rs.Checklist:
structure.rs, regeneratedgrovedb-structure.json, and checked the structure viewer link posted on this pull requestFor repository code-owners and collaborators only
🤖 Co-authored by Claudius the Magnificent AI Agent
PR Hygiene ·
9774c72/skip-botsproceeds without the ones not yet reported/self-reviewed.devcontainer/devcontainer-build.json,.pnp.cjs,.yarn/cache/@types-node-npm-20.19.30-e3d3d7af6e-4a25e5cbcd.zipand 42 more) — QuantumExplorer or shumkovdashmate(packages/dashmate/docs/installation.md,packages/dashmate/package.json) — ktechmidas or shumkovjs-wasm-sdk(packages/js-dash-sdk/README.md,packages/js-dash-sdk/package.json,packages/js-evo-sdk/README.mdand 10 more) — shumkovkotlin-sdk(packages/kotlin-sdk/sdk/src/main/kotlin/org/dashfoundation/dashsdk/errors/DashSdkError.kt,packages/kotlin-sdk/sdk/src/test/kotlin/org/dashfoundation/dashsdk/errors/DashSdkErrorTest.kt) — HashEngineeringrust-dapi— you own itdpp(packages/rs-dpp/Cargo.toml,packages/rs-dpp/src/address_funds/platform_address.rs,packages/rs-dpp/src/bls/bls_signatures.rsand 25 more) — QuantumExplorer or shumkovrs-drive-abci(packages/rs-drive-abci/src/abci/error.rs,packages/rs-drive-abci/src/error/execution.rs,packages/rs-drive-abci/src/error/mod.rsand 42 more) — QuantumExplorer or shumkovrs-platform-wallet-ffi(packages/rs-platform-wallet-ffi/ERROR_CODE_REGISTRY.md,packages/rs-platform-wallet-ffi/src/dashpay.rs,packages/rs-platform-wallet-ffi/src/derivation.rsand 14 more) — HashEngineering or ZocoLini or llbartekll or romchornyiwallet-storage— you own itrs-platform-wallet(packages/rs-platform-wallet/Cargo.toml,packages/rs-platform-wallet/examples/dpns_marketplace_testnet.rs,packages/rs-platform-wallet/src/changeset/core_bridge.rsand 33 more) — HashEngineering or ZocoLini or llbartekll or romchornyirust-sdk-ffi— you own itrust-sdk— you own itswift-sdk(packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletManagerSPV.swift,packages/swift-sdk/Sources/SwiftDashSDK/PlatformWallet/PlatformWalletResult.swift,packages/swift-sdk/SwiftTests/SwiftDashSDKTests/ErrorHandlingTests.swiftand 1 more) — llbartekll or romchornyiWhen every merge requirement is met, the
PR Hygienecheck passes. Reviewer limits do not block merging; other required GitHub checks and protections still apply.